Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GEIQ: Afficher le diagnostic GEIQ d'une candidature acceptée dans le passé [GEN-1701] #4456

Merged
merged 5 commits into from
Jul 31, 2024

Conversation

tonial
Copy link
Contributor

@tonial tonial commented Jul 22, 2024

🤔 Pourquoi ?

Pour permettre de conserver l'information même quand le diagnostic n'est plus valide

Notes pour discussion métier :

employeur et candidat

on prend dans l'ordre de présence

  • le diag de la candidature
  • le diag le plus récent fait par un prescripteur habilité
  • le diag le plus récent fait par l'entreprise destinatrice

=> tester le cas un diag expiré d'un prescritpeur et un diag valid de la structure
On ne veut pas d'abord le diag valide ?

prescripteur

on prend le diag valide pour ce candidat fait par un presripteur habitilité (s'il existe)

mais on ne prend pas un diag d'un prescripteur habilité expiré ?
sur une candidature acceptée, on n'affiche pas non plus le diag de l'entreprise ?

🍰 Comment ?

Décrivez en quelques mots la solution retenue et mise en oeuvre, les difficultés ou problèmes rencontrés. Attirez l'attention sur les décisions d'architecture ou de conception importantes.

🚨 À vérifier

  • Mettre à jour le CHANGELOG_breaking_changes.md ?

🏝️ Comment tester

Les instructions pour reproduire le problème, les profils de test, le parcours spécifique à utiliser, etc. Si vous disposez d'une recette jetable, mettre l'URL pour tester dans cette partie.

💻 Captures d'écran

@tonial tonial self-assigned this Jul 22, 2024
@tonial tonial changed the title GEIQ: Afficher le diagnostic GEIC d'une candidature accpetée dans le passé GEIQ: Afficher le diagnostic GEIC d'une candidature acceptée dans le passé Jul 23, 2024
@francoisfreitag francoisfreitag changed the title GEIQ: Afficher le diagnostic GEIC d'une candidature acceptée dans le passé GEIQ: Afficher le diagnostic GEIQ d'une candidature acceptée dans le passé Jul 24, 2024
Copy link
Contributor

@francoisfreitag francoisfreitag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je vois que la vue itou.apply.views.process_views.details_for_prescriber se base sur les diags d’éligibilité GEIQ actuels du candidat. Ne devrait-elle pas se baser sur le diagnostic de la candidature (job_app.geiq_eligibility_diagnosis) s’il existe ?

Peut-être à mutualiser avec _get_geiq_eligibility_diagnosis_for_company qui est utilisé pour le candidat et l’entreprise qui l’embauche ?

itou/eligibility/models/geiq.py Outdated Show resolved Hide resolved
tests/www/apply/test_geiq.py Outdated Show resolved Hide resolved
@tonial
Copy link
Contributor Author

tonial commented Jul 25, 2024

Alors ta remarque sur _get_geiq_eligibility_diagnosis_for_company est pertinante, mais je vois que la logique entre les vue est différentes :

  • Pour un prescripteur, on veut afficher le diagnostic de la candidature s'il existe (c'est à corriger) et sinon un diagnostic valide
  • Pour les autres, on affiche le diagnostic de la candidature s'il existe ou le plus récent des expirés

Par contre je suis en train de regarder les tests de cette vue, et c'est à revoir entièrement, tous les tests regardent la vue "details_for_company" même quand il y a écrit "as_prescriber" dans le nom du test.

Je sais ce que je peux faire ce matin 🙄

@tonial
Copy link
Contributor Author

tonial commented Jul 25, 2024

@francoisfreitag j'ai revu entièrement les tests pour cette histoire de choix du diagnostic affiché aux prescripteurs différent de celui affiché aux autres utilisateurs.

Je ne suis pas 100% convaincu

Je me demande si une meilleurs approche ne serait pas de faire d'un côté la vérification du rendu du template geiq_diagnosis_details.html en fonctions des cas : qui regarde, et quel type de diag (diag valide / expiré / de montant nul)
Puis de vérifier les différents cas comme dans les tests modifié, mais en regardant juste quel diagnostic est récupéré dans le contexte.

Mais bon, ça risque de faire encore plus de tests, donc pas sur que ce soit mieux.

@francoisfreitag francoisfreitag changed the title GEIQ: Afficher le diagnostic GEIQ d'une candidature acceptée dans le passé GEIQ: Afficher le diagnostic GEIQ d'une candidature acceptée dans le passé [GEN-1701] Jul 29, 2024
Copy link
Contributor

@francoisfreitag francoisfreitag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je vois quelques questions dans la description de la PR qui mériterait une réponse, notamment :

On ne veut pas d'abord le diag valide ?

Est-ce qu’on veut le diag le plus récent ? Par exemple, un prescripteur fait un diag il y a un an, le GEIQ fait un diag il y a 9 mois. Est-ce qu’on affiche le diag du prescripteur ou celui du GEIQ ?



@freeze_time("2024-02-15")
class JobApplicationGEIQEligibilityDetailsTest(TestCase):
@pytest.mark.ignore_unknown_variable_template_error("with_matomo_event")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Est-il possible de corriger le problème au lieu de l’ignorer ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je dois avouer que je n'ai pas envie de le faire dans cette PR, mais que c'est prévu pour après :)

tests/www/apply/test_geiq.py Outdated Show resolved Hide resolved
tests/www/apply/test_geiq.py Outdated Show resolved Hide resolved
The eligibility is confirmed as soon as the amount is not null.
What we want to know is if it grants eligibility today
@@ -1,13 +1,13 @@
{% if diagnosis.is_valid %}
{% if diagnosis.eligibility_confirmed %}
{% if diagnosis.is_valid or job_application.state.is_accepted and diagnosis %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{% if diagnosis.is_valid or job_application.state.is_accepted and diagnosis %}
{% if diagnosis.is_valid or job_application.state.is_accepted %}

Copy link
Contributor Author

@tonial tonial Jul 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

N'est-il pas possible d'avoir une candidature acceptée vers un GEIQ sans diagnostic associé ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Si. Mais même sans diagnostic, je pense qu'on souhaite mettre Éligibilité public prioritaire GEIQ non confirmée pour une candidature acceptée sans diag ? (mais il faudrait également adapter d'autres else/elif du template.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C'est déjà traité alors : on passe dans le else en bas et à présent on marque Éligibilité public prioritaire GEIQ non confirmée même si ce n'est pas une entreprise 😎

@tonial tonial force-pushed the alaurent/geiq branch 2 times, most recently from a07e3dd to 88b7e6c Compare July 30, 2024 12:38
@@ -1,13 +1,13 @@
{% if diagnosis.is_valid %}
{% if diagnosis.eligibility_confirmed %}
{% if diagnosis.is_valid or job_application.state.is_accepted and diagnosis %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Si. Mais même sans diagnostic, je pense qu'on souhaite mettre Éligibilité public prioritaire GEIQ non confirmée pour une candidature acceptée sans diag ? (mais il faudrait également adapter d'autres else/elif du template.

itou/www/apply/views/process_views.py Outdated Show resolved Hide resolved
itou/www/apply/views/process_views.py Outdated Show resolved Hide resolved
itou/www/apply/views/process_views.py Outdated Show resolved Hide resolved
itou/www/apply/views/process_views.py Outdated Show resolved Hide resolved
Copy link
Contributor

@francoisfreitag francoisfreitag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heureusement qu’il y a le harnais de test pour tout expliquer aux prochains devs !!

itou/www/apply/views/process_views.py Show resolved Hide resolved
itou/www/apply/views/process_views.py Outdated Show resolved Hide resolved
itou/www/apply/views/process_views.py Outdated Show resolved Hide resolved
itou/www/apply/views/process_views.py Outdated Show resolved Hide resolved
We didn't test the view for job seekers or prescribers...
@tonial tonial added this pull request to the merge queue Jul 31, 2024
Merged via the queue into master with commit 41c6403 Jul 31, 2024
11 checks passed
@tonial tonial deleted the alaurent/geiq branch July 31, 2024 03:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants